CORE-764: new sentry logger #77
Conversation
| @@ -1,6 +1,6 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`ToggleButtonGroup matches snapshot with selectionMode #selectionMode 1`] = ` | |||
| exports[`ToggleButtonGroup matches snapshot with selectionMode multiple 1`] = ` | |||
There was a problem hiding this comment.
Fix test descriptions
| @@ -30,7 +30,8 @@ describe('ToggleButtonGroup', () => { | |||
| selectionMode | |||
| ${'multiple'} | |||
| ${'single'} | |||
| `(`matches snapshot with selectionMode #selectionMode`, ({selectionMode}) => { | |||
| ${undefined} | |||
There was a problem hiding this comment.
Covering line 20 of ToggleButtonGroup file.
Dantemss
requested review from
a team and
jmonteroa05
and removed request for
a team
src/sentryLogger/sentryLog.ts
Outdated
| * More info: https://develop.sentry.dev/sdk/data-model/event-payloads/breadcrumbs/ | ||
| */ | ||
|
|
||
| const serializeLevel = (level: Level): string => |
There was a problem hiding this comment.
you could make this
| const serializeLevel = (level: Level): string => | |
| const serializeLevel = (level: Level): SeverityLevel => |
src/sentryLogger/sentryLog.ts
Outdated
| return { | ||
| type: (type?? '') as string, | ||
| level: serializeLevel(level) as SeverityLevel, | ||
| "event_id": (event_id ?? '') as string, |
There was a problem hiding this comment.
i don't see this as an allowed parameters, is there other documentation that needs to be linked to?
There was a problem hiding this comment.
You're right. I took it from code. addBreadcrumb is expecting:
type?: string;
level?: Severity | SeverityLevel;
event_id?: string;
category?: string;
message?: string;
data?: {
[key: string]: any;
};
timestamp?: number;
That is why I leave it there
There was a problem hiding this comment.
I'll remove it to keep it faithful to the documentation
src/sentryLogger/sentryLog.ts
Outdated
| type: (type?? '') as string, | ||
| level: serializeLevel(level) as SeverityLevel, | ||
| "event_id": (event_id ?? '') as string, | ||
| category: (category ?? '') as string, |
There was a problem hiding this comment.
you should do typeof checks on all of these instead of just casting them, the input interface would allow them to be arrays or objects which wouldn't work
src/sentryLogger/sentryLog.ts
Outdated
| timestamp?: number; | ||
| data?: { [key: string]: any }; | ||
| } => { | ||
| const { type, event_id, category, message, timestamp, data } = breadcrumb; |
There was a problem hiding this comment.
you could take a rest parameter here and append the remaining stuff in the event to the data key. you'll have to do something to flatten it though because i think it only supports key value pairs. maybe something like this https://stackoverflow.com/a/74769002/14809536
| value && typeof value === typeValue ? value : defaultValue | ||
| ; | ||
|
|
||
| const flattenObj = (obj: { [x: string]: any; }) => |
There was a problem hiding this comment.
when copying red-code from stack overflow i like to put a link to the SO answer in a comment above the function
src/sentryLogger/sentryLog.ts
Outdated
| const { type, category, message, timestamp, ...rest } = breadcrumb; | ||
|
|
||
| return { | ||
| type: checkTypeOf(type, 'string', ''), |
There was a problem hiding this comment.
it looks like breadcrumb type means stuff https://develop.sentry.dev/sdk/data-model/event-payloads/breadcrumbs/#breadcrumb-types and each one has a different schema for the other properties. based on what we have here so far, we may just want to submit only 'default' breadcrumbs and worry about adding support for other types later.
There was a problem hiding this comment.
Reading type 'default' , I see that is expecting data as undefined. We need a breadcrumb with category="log" and avoid passing type in order to send a data key. Thanks for highlighting this info to me.
src/sentryLogger/sentryLog.ts
Outdated
| const checkTypeOf = ( | ||
| value: JsonCompatibleStruct | JsonCompatibleValue | JsonCompatibleArray, | ||
| typeValue: string, | ||
| defaultValue: any, | ||
| ) => | ||
| value && typeof value === typeValue ? value : defaultValue | ||
| ; |
There was a problem hiding this comment.
this seems to work fine for what you're doing, but it doesn't have a type safe response. a generic helper would look like this although its of course much simpler if you make it string specific. you're also not really saving yourself that many characters where you're calling this, you could just put all the ternaries inline.
Card: https://openstax.atlassian.net/browse/CORE-764
First step: implement a logger that logs to the sentry breadcrumbs